-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't show the Download App Banner until Past Sign in #26182
Conversation
@jjcoffee please prioritize this review as it's a high priority item! 🙇♂️ |
@jjcoffee could you please prioritize this PR review? 🙏 E/E issue reads:
|
Reviewer Checklist
Screenshots/VideosWebchrome-desktop-2023-08-29_17.08.23.mp4Mobile Web - Chromeandroid-chrome.mp4Mobile Web - Safariios-safari-2023-08-29_16.12.02.mp4Desktopmac-desktop-2023-08-29_16.17.24.mp4iOSios-native-2023-08-29_15.47.11.mp4Androidandroid-native-2023-08-29_17.00.41.mp4 |
@grgia Just a nitpick: test steps need to updated to state the banner only appears after sign in. Also, not sure if it's a biggie but there are missing screens for platforms other than iOS mWeb (maybe we're skipping this to get it done quickly?). |
@jjcoffee I have them, adding now (just wrapped up lunch)! |
src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js
Outdated
Show resolved
Hide resolved
@grgia Sorry another nitpick on the tests, there's still this line which is no longer relevant: |
@grgia Just the tweak to the tests and now some lint complaints and then we're good to go, I think! |
@jjcoffee looking now! |
@grgia I think the test step |
Oh that's correct, I'm removing that |
@neil-marcellini Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying my best to understand this but finding it pretty confusing. Please update the test steps and provide a more thorough description of the problems and solutions solved by this PR.
Specifically I'm not understanding how this prevents showing the banner on the saastr login page.
src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js
Outdated
Show resolved
Hide resolved
src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js
Outdated
Show resolved
Hide resolved
src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you for updating the PR description, code, and walking me through this.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.59-0 🚀
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.60-0 🚀
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.60-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.59-5 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.60-3 🚀
|
Details
Problem:
We originally showed this banner to all mWeb users either on the sign-in page or past sign in. As part of the SaaStr project, it was decided that the focus will be 1. Getting more users signed up and then 2. Getting users to download the app.
Solution
We will not show the Download App Banner on any sign on page (this includes our landing page and /saastr). Instead, we will show the banner after the user has logged in, and before the global create menu is opened for first time users.
Follow up for #25418
Changes:
See video:
Screen.Recording.2023-08-29.at.12.38.07.PM.mov
Fixed Issues
#25820
$ https://github.com/Expensify/Expensify/issues/310804
PROPOSAL:
Tests
For the following platforms: NATIVE, WEB, DESKTOP
mWEB
mWEB IOS
mWEB Android
Offline tests
QA Steps
For the following platforms: NATIVE, WEB, DESKTOP
mWEB
mWEB IOS
mWEB Android
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
No Banner
Screen.Recording.2023-08-29.at.1.36.58.PM.mov
Mobile Web - Chrome
Screen.Recording.2023-08-29.at.1.33.39.PM.mov
Mobile Web - Safari
Screen.Recording.2023-08-29.at.12.38.07.PM.mov
Desktop
No Banner
Screen.Recording.2023-08-29.at.1.36.00.PM.mov
iOS
No Banner
Screen.Recording.2023-08-29.at.1.38.26.PM.mov
Android
No Banner
Emulator loading